Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize text before making repr_html #3378

Merged
merged 12 commits into from
Oct 16, 2019
Merged

Conversation

stephenworsley
Copy link
Contributor

Addresses #3377. May be relevant to #3313.
All text which is taken from a cube and put into its repr_html is now passed through html.escape() first.

@stephenworsley stephenworsley changed the title Sanitize text before making html_repr Sanitize text before making repr_html Aug 23, 2019
Copy link
Member

@DPeterK DPeterK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking to sanitize the inputs for stray html. I think we could simplify the approach and I've made comments to that effect below. Again we're also missing tests for these changes. It would be good to see a test for every string element that gets added to the html repr confirming that stray html is being sanitized.

lib/iris/experimental/representation.py Outdated Show resolved Hide resolved
lib/iris/experimental/representation.py Outdated Show resolved Hide resolved
lib/iris/experimental/representation.py Outdated Show resolved Hide resolved
lib/iris/experimental/representation.py Outdated Show resolved Hide resolved
@stephenworsley
Copy link
Contributor Author

So it looks like the html module was added to python in version 3.2 so this is not compatible with 2.7. That said, the escape function isn't that complicated as it just involves a couple of string replace operations (In the form i'm using, it could be written in about 5 lines).
I think I may rewrite the escape function instead of importing it.

lib/iris/experimental/representation.py Outdated Show resolved Hide resolved
lib/iris/experimental/representation.py Outdated Show resolved Hide resolved
@DPeterK
Copy link
Member

DPeterK commented Sep 2, 2019

I think I may rewrite the escape function instead of importing it.

👍 Support this decision (with the expectation that it will be reversed once Iris goes Python 3+ only!)

@lbdreyer
Copy link
Member

@stephenworsley I've moved this PR back into to the "In progress" column as it looks like this is still requiring tests

# Conflicts:
#	lib/iris/experimental/representation.py
#	lib/iris/tests/stock/__init__.py
@bjlittle
Copy link
Member

bjlittle commented Oct 3, 2019

@stephenworsley @lbdreyer Is this PR making the cut for 2.3.0?

If so, it needs a bit of ❤️ and should now target the 2.3.x branch, not master.

Otherwise, let's target 3.0.0

@lbdreyer
Copy link
Member

lbdreyer commented Oct 4, 2019

@stephenworsley @lbdreyer Is this PR making the cut for 2.3.0?

If so, it needs a bit of heart and should now target the 2.3.x branch, not master.

Otherwise, let's target 3.0.0

I think it's a bit too late, unfortunately. And I'm a little unsure about the approach.
I think it is better to wait for Iris 3

@stephenworsley
Copy link
Contributor Author

I've been having a look at how this interacts with #3313. From what I can tell, it's looking somewhat less trivial now, especially with the way the tests for CubeListRepresentation are set up. So I would agree with @lbdreyer here, it's probably one for 3.0.

@stephenworsley stephenworsley force-pushed the clean_html branch 2 times, most recently from c20036f to 0967c12 Compare October 10, 2019 15:40
@pp-mo
Copy link
Member

pp-mo commented Oct 15, 2019

Hi @stephenworsley
All looks good to me,
though I'm not merging until I'm a bit happier with my own understanding of the original code, which is a bit obscure IMO.

However, I unfortunately found that none of the tests in iris.tests.unit.experimental.representation have actually been running .
I think because it has been missing an __init__.py in the containing folder.

So if you examine the Travis log + search for "representation", it is running iris.tests.integration.experimental.test_CubeRepresentation , but not iris.tests.unit.experimental.test_CubeRepresentation, or iris.tests.unit.experimental.test_CubeListRepresentation.

Naturally, this means they do not pass 🤕 .
Arrrgh.

@pp-mo
Copy link
Member

pp-mo commented Oct 15, 2019

Re: stephenworsley#5
I think that fixes the above : please review + merge if you think correct @stephenworsley

OWOK, I think 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants